-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[file_packager.py] Add --export-es6 to file_packager.py #24737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c4a662f
to
deac6ec
Compare
@sbc100 could you please take a look ? |
Can you explain a little more in the PR description exactly what |
I have added some use case description. As described in the related issue we are just trying to make the dynamic loading of large necessary dependencies inside web workers run from react application. Together with XHR lazy loading this seems to be only valid solution to our problem that might solve our problem. |
1b74c3e
to
05f9693
Compare
@sbc100 what do you think about it now? |
47dcb43
to
e2b3fc9
Compare
a83bbfe
to
58e06be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we still need a test for this.
And might be worth adding to the changelog.
Otherwise lgtm.
38f054e
to
0a72d80
Compare
I have added two simple tests - from what I have seen |
1e7790c
to
149e8b3
Compare
b011fb3
to
fb98269
Compare
bc34582
to
b8b0baa
Compare
8495023
to
7305ad0
Compare
I manage to solve the issue, apperantly we do not have any tests with --no-node support? Is it possible to run test from web context at all to test if fetch works insteads of nodes method? Now happy path works but promise is not rejected where it should be most likely - there is were I need that PR: #24871 |
e59ffa9
to
6f10cbb
Compare
c08fbb5
to
ad3b54c
Compare
fix python format apply more ruff fixes fix pipeline revert file to the most actual version and apply changes add modularize to options class add utilization of EXPORT_NAME with modularize option simplify declaration of the modularize function arg documentation address @sbc100 review Fix codegen new line formatting Add file_packager tests and changelog mention fix some stylistic issues gichange file_packager modularize test simplyfi tests fix test rename to export_es6 async function promise behaviour cleanup test address @sbc100 review simplify the test rename promise handlers introduce createRequire in es6 mode alternative way to import require
ad3b54c
to
be712ca
Compare
test/test_other.py
Outdated
.catch((cause) => Promise.reject(cause)) | ||
.then(() => { | ||
module._test_fun(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the .catch
here. Doing return loadDataFile(..
should be enough to reject the promise if loadDataFile fails/rejects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think just using top-level-await might be more clear here:
var module = await loadModule();
await loadDataFile(module):
module._test_fun();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will apply that later, I have changed some logic to be more universal and to allow catching thrown error, take a look please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, if I shouldn't add some throw error method to clear run dependencies - or maybe generate separate stub that allows to do that if we want ourselfs in external catch block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, if I shouldn't add some throw error method to clear run dependencies - or maybe generate separate stub that allows to do that if we want ourselfs in external catch block
It could be other PR tho, I have just observed anoying behaviour that when it fails, main module constantly spams run dependencies message, being able to clean those to graceful escape error would be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that removing the run dependency is the way to fix that.. since that would result in the module running. If a dependency fails we want to block the running forever I think.. not just ignore it. But I think the existing code might not do that :(
tools/file_packager.py
Outdated
|
||
if not options.export_es6: | ||
ret += ''' | ||
(async () => {''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is async
being added here for the non-export-es6 case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I wanted to wait for the loadPackage which was made async in one of yours PR chain
tools/file_packager.py
Outdated
@@ -1126,9 +1152,15 @@ def generate_js(data_target, data_files, metadata): | |||
else: | |||
ret += ''' | |||
} | |||
loadPackage(%s);\n''' % json.dumps(metadata) | |||
await loadPackage(%s);\n''' % json.dumps(metadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the very end of the function then you can just do return loadPackage
I think?
(doing return
instead of await
might also mean you don't need to make the outer function async
).
test/test_other.py
Outdated
.catch((cause) => Promise.reject(cause)) | ||
.then(() => { | ||
module._test_fun(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that removing the run dependency is the way to fix that.. since that would result in the module running. If a dependency fails we want to block the running forever I think.. not just ignore it. But I think the existing code might not do that :(
c0368f8
to
529e4a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % final comments
} else { | ||
fetched = data; | ||
} | ||
})%s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there is no current error handling here, for this fetch. It looks like the code is trying to do a pre-fetch and maybe we can just ignore failures (like we currently do) for the pre-fetch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case this block could be reverted
catch_case = ''' | ||
.catch((error) => { | ||
loadDataReject(error); | ||
})''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about doing:
catch_handler = ''
if export_es6:
catch_handler = ''' ... '''
Then you don't need repeated (catch_case if options.export_es6 else '')
below.
} | ||
}; | ||
var files = metadata['files']; | ||
for (var i = 0; i < files.length; ++i) { | ||
new DataRequest(files[i]['start'], files[i]['end'], files[i]['audio'] || 0).open('GET', files[i]['filename']); | ||
}\n''' % (create_preloaded if options.use_preload_plugins else create_data) | ||
}\n''' % (create_preloaded if options.use_preload_plugins else create_data, ready_promise if options.export_es6 else '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
finish_handler = create_preloaded if options.use_preload_plugins else create_data
if export_es6:
finish_handler += '\nloadDataResolve();'
Then use finish_handler
here in the string template.
I have added modularize option flag to
file_packager.py
script, like in main js generatorI think it will be helpful since in modern world of react it is inconvinent to add generated files as
<script/>
tags.This pull request introduces a new
--export-es6
option to thefile_packager.py
tool, enabling the generation of modularized JavaScript output. It enables to import generated JS loading stub into ES6 environment. The main module which will take effect of script execution is now passed as script argument.Use scenario:
Fixes: #24504